Do not use std::aligned_storage#47
Conversation
[why] We should verify that SmallVector actually places its elements according to their alignment requirements. As it turns out, it does. :)
[why] std::aligned_storage is deprecated since C++23, see: https://stackoverflow.com/questions/71828288/why-is-stdaligned-storage-to-be-deprecated-in-c23-and-what-to-use-instead [how] Use alignas() for the internal buffer and specify the alignment explicitly when calling new for an allocated buffer. As it turns out, an aligned new[] has to be deallocated with an aligned delete[].
alt-graph
requested review from
Seim2k17,
Copilot,
ohensler,
soerengrunewald and
zykure
There was a problem hiding this comment.
Pull request overview
This PR updates gul17::SmallVector to avoid using std::aligned_storage (deprecated in C++23) by switching to an alignas(...)-aligned internal byte buffer and using aligned new[] / delete[] for heap storage. It also adds a unit test to validate alignment after growth and shrink_to_fit().
Changes:
- Replace
std::aligned_storagewith analignas(ValueType)internalstd::bytebuffer and aligned heap allocation/deallocation. - Add an alignment regression test that exercises
push_back()growth andshrink_to_fit()transitions. - Update changelog/copyright headers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
include/gul17/SmallVector.h |
Reworks internal/heap storage to avoid std::aligned_storage and uses aligned new[]/delete[]. |
tests/test_SmallVector.cc |
Adds a new template test to verify element alignment and contiguity across growth/shrink. |
data/doxygen.h |
Updates the “Unreleased” changelog section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
[why] The function could leave data_ptr_ dangling after the call. This was clearly documented and logically OK within the class, but could raise suspicion. [how] Explicitly document that the function may leave the class in a state in which the class invariants are violated. If deallocation takes place, assign nullptr to data_ptr_ so that there is a better chance to find misuses.
[why]
MSVC mistakes the statement
allocation = new (alignment) std::byte[new_capacity * sizeof(ValueType)];
for a placement new even if alignment is of the type std::align_val_t,
and therefore generates a compilation error.
[how]
Use the low-level form of new directly like this:
auto* ptr = static_cast<std::byte*>(
::operator new[](num_elements * sizeof(ValueType), alignment));
Because this is ugly, extract both allocation and deallocation into
static member functions.
[why] Having a private member function that breaks the class invariants may be confusing, and it does not make things any clearer than having the two lines directly in the code.
alt-graph
force-pushed
the
cleanup/do_not_use_std_aligned_storage
branch
from
f3533f0 to
7c0ab98
Compare
Finii
left a comment
Finii
left a comment
There was a problem hiding this comment.
I'm not too familiar with this alignment stuff, esp because I'm used to x86 which is so forgiving with alignment).
But I guess this looks good.
I have some minor comments, though.
And maybe we should run this at least on arm (do we?) because arm does not tolerate alignment errors and bus-errors out (instead of just creating a runtime penalty as on x86).
Apart from the hardware platform I also have no clue how (older) MSVC handle this 🤔
According to https://github.com/actions/runner-images |
Good idea. It builds and tests fine on MacOS-aarch64. ✅
It also builds and tests fine on my "ancient" x86-64 Visual Studio 2019 installation. ✅ |
|
What I do not really understand, esp after reading the linked SO thread, by do we stick to - alignas(ValueType)
- std::array<std::byte, in_capacity * sizeof(ValueType)> internal_array_;
-
- /// Pointer to internal or external contiguous data storage.
- std::byte* data_ptr_{ internal_array_.data() };
-
- static std::byte* allocate_space_for_elements(std::size_t num_elements)
- {
- return static_cast<std::byte*>(
- ::operator new[](num_elements * sizeof(ValueType), alignment));
- }
+ std::array<ValueType, in_capacity> internal_array_;
+
+ /// Pointer to internal or external contiguous data storage.
+ ValueType* data_ptr_{ internal_array_.data() };
+
+ static ValueType* allocate_space_for_elements(std::size_t num_elements)
+ {
+ return static_cast<ValueType*>(
+ ::operator new[](num_elements * sizeof(ValueType), alignment));
+ }And then we can drop all the |
[why]
The code actually becomes more readable by using the expressions
alignof(ValueType) or std::align_val_t{ alignof(ValueType) } directly.
|
NICE |
[how] If we store data_ptr as a ValueType* instead of std::byte* and access the internal array only via an accessor function get_internal_array_pointer(), we can limit the need for reinterpret_cast to that accessor function (the internal array is still an array of std::byte). Proposed-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Because I was too lazy. ;) Yes, I guess it is worth it. Commit a3f3662 removes most of the |
[why] It makes the code more compact and more readable.
|
@Finii: I have added two more commits after your approval. Please let me know if you want to have another look. |
Both look very good 👍 I struggle a bit with understanding the const correctness of |
|
Thanks a lot, @Finii ! |
[why]
std::aligned_storageis deprecated since C++23, see: https://stackoverflow.com/questions/71828288/why-is-stdaligned-storage-to-be-deprecated-in-c23-and-what-to-use-instead[how]
Use
alignas()for the internal buffer and specify the alignment explicitly when callingnewfor an allocated buffer. As it turns out, an alignednew[]has to be deallocated with an aligneddelete[], so we have to change that handling, too. We also had no unit test for the alignment requirements, so we add one.